security: baseband_guard: fix two bugs in is_protected_blkdev causing…#78
Open
leegarchat wants to merge 1 commit intovc-teahouse:mainfrom
Open
security: baseband_guard: fix two bugs in is_protected_blkdev causing…#78leegarchat wants to merge 1 commit intovc-teahouse:mainfrom
leegarchat wants to merge 1 commit intovc-teahouse:mainfrom
Conversation
… panic and dead code
Two independent bugs in is_protected_blkdev(), called from the
bb_inode_rename and bb_inode_setattr LSM hooks.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Bug 1 — Inverted NULL guard (all protection logic is dead code)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
if (!IS_ERR_OR_NULL(dentry)) /* BUG: ! inverts the guard */
return 0;
IS_ERR_OR_NULL() returns true for NULL or err-pointer dentries.
Negating it means the condition is true for every valid (non-NULL,
non-error) dentry — i.e. every real rename or setattr call. The
function returned 0 immediately, making every line below it unreachable
dead code. Block device and symlink rename / setattr protection was
silently disabled for all callers.
Fix:
if (IS_ERR_OR_NULL(dentry))
return 0;
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Bug 2 — page_get_link() called on fast symlinks → NULL deref → kernel panic
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
const char *link = inode->i_op->get_link(dentry, inode, &done);
The code calls get_link() to resolve the symlink target and verify it
does not point into /dev/block/by-name. On most filesystems this
dispatches to page_get_link(), which reads the target from the inode's
page-cache mapping.
"Fast symlinks" (tmpfs, ramfs, shmem — target length ≤ ~62 bytes on
64-bit) store the target string directly in inode->i_link. They do not
set up a page-cache mapping; inode->i_mapping->a_ops is left NULL.
page_get_link() → __page_get_link() unconditionally dereferences
inode->i_mapping->a_ops, causing:
Unable to handle kernel NULL pointer dereference at 0x0
__page_get_link+0xb4/0x144
page_get_link+0x18/0x48
bb_inode_rename+0x134/0x264
security_inode_rename+0x90/0x100
vfs_rename+0x154/0x514
Kernel panic — not syncing: Oops: Fatal exception
Trigger: rename of any short symlink on rootfs/tmpfs/ramfs, e.g.:
mv /etc /etc_link (AnyKernel3 setup_mountpoint on recovery rootfs)
mv /tmp /tmp_bak
rename("/system", "/system_orig", ...)
Symlinks in /dev/block/by-name always use long absolute targets
(e.g. /dev/block/sde37) and are never fast symlinks, so they are
unaffected. The protection goal — blocking renames that redirect
by-name entries — is 100% preserved by this fix.
Fix: check inode->i_link before calling get_link(). Fast symlinks have
inode->i_link set; the target string is already in memory and can be
used directly. Slow symlinks have inode->i_link == NULL and fall through
to the original get_link() path unchanged.
if (inode->i_link) {
symlink_target_link = inode->i_link; /* inline target, no page fault */
} else {
symlink_target_link = inode->i_op->get_link(dentry, inode, &done);
}
Note: Bug 1 is what prevented the panic in binaries compiled from this
source after the inverted guard was introduced as an emergency workaround.
Fixing Bug 1 (restoring correctness) re-exposes Bug 2 — both must be
fixed together.
Reproducer (before fix, with correct Bug-1 guard):
insmod baseband_guard.ko
mv /etc /etc_link → instant kernel panic
(after fix) → rename allowed, no panic
Reported-by: LeeGarChat
Collaborator
|
... did it actually exist? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… panic and dead code
Two independent bugs in is_protected_blkdev(), called from the bb_inode_rename and bb_inode_setattr LSM hooks.
━━━━━━━━━━━━━━━━━━━━━━
Bug 1 — Inverted NULL guard (all protection logic is dead code)
━━━━━━━━━━━━━━━━━━━━━━
if (!IS_ERR_OR_NULL(dentry)) /* BUG: ! inverts the guard */
return 0;
IS_ERR_OR_NULL() returns true for NULL or err-pointer dentries. Negating it means the condition is true for every valid (non-NULL, non-error) dentry — i.e. every real rename or setattr call. The function returned 0 immediately, making every line below it unreachable dead code. Block device and symlink rename / setattr protection was silently disabled for all callers.
Fix:
if (IS_ERR_OR_NULL(dentry))
return 0;
━━━━━━━━━━━━━━━━━━━━━━
Bug 2 — page_get_link() called on fast symlinks → NULL deref → kernel panic
━━━━━━━━━━━━━━━━━━━━━━
const char *link = inode->i_op->get_link(dentry, inode, &done);
The code calls get_link() to resolve the symlink target and verify it does not point into /dev/block/by-name. On most filesystems this dispatches to page_get_link(), which reads the target from the inode's page-cache mapping.
"Fast symlinks" (tmpfs, ramfs, shmem — target length ≤ ~62 bytes on 64-bit) store the target string directly in inode->i_link. They do not set up a page-cache mapping; inode->i_mapping->a_ops is left NULL.
page_get_link() → __page_get_link() unconditionally dereferences inode->i_mapping->a_ops, causing:
Unable to handle kernel NULL pointer dereference at 0x0
__page_get_link+0xb4/0x144
page_get_link+0x18/0x48
bb_inode_rename+0x134/0x264
security_inode_rename+0x90/0x100
vfs_rename+0x154/0x514
Kernel panic — not syncing: Oops: Fatal exception
Trigger: rename of any short symlink on rootfs/tmpfs/ramfs, e.g.:
mv /etc /etc_link (AnyKernel3 setup_mountpoint on recovery rootfs)
mv /tmp /tmp_bak
rename("/system", "/system_orig", ...)
Symlinks in /dev/block/by-name always use long absolute targets (e.g. /dev/block/sde37) and are never fast symlinks, so they are unaffected. The protection goal — blocking renames that redirect by-name entries — is 100% preserved by this fix.
Fix: check inode->i_link before calling get_link(). Fast symlinks have inode->i_link set; the target string is already in memory and can be used directly. Slow symlinks have inode->i_link == NULL and fall through to the original get_link() path unchanged.
if (inode->i_link) {
symlink_target_link = inode->i_link; /* inline target, no page fault */
} else {
symlink_target_link = inode->i_op->get_link(dentry, inode, &done);
}
Note: Bug 1 is what prevented the panic in binaries compiled from this source after the inverted guard was introduced as an emergency workaround. Fixing Bug 1 (restoring correctness) re-exposes Bug 2 — both must be fixed together.
Reproducer (before fix, with correct Bug-1 guard):
insmod baseband_guard.ko
mv /etc /etc_link → instant kernel panic
(after fix) → rename allowed, no panic
Reported-by: LeeGarChat